-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[PowerPC][CodeGen] Expand ISD::AssertNoFPClass for ppc_fp128 #152357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-backend-powerpc Author: Amy Kwan (amy-kwan) Changes780054d added support for This ISD node can be used with the
Thus, this patch aims to add support for the expand so we no longer assert. This fixes #151375. Full diff: https://github.com/llvm/llvm-project/pull/152357.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
index 2cad36eff9c88..8140c0e771a7b 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
@@ -1551,6 +1551,7 @@ void DAGTypeLegalizer::ExpandFloatResult(SDNode *N, unsigned ResNo) {
case ISD::VAARG: ExpandRes_VAARG(N, Lo, Hi); break;
case ISD::ConstantFP: ExpandFloatRes_ConstantFP(N, Lo, Hi); break;
+ case ISD::AssertNoFPClass: ExpandFloatRes_AssertNoFPClass(N, Lo, Hi); break;
case ISD::FABS: ExpandFloatRes_FABS(N, Lo, Hi); break;
case ISD::STRICT_FMINNUM:
case ISD::FMINNUM: ExpandFloatRes_FMINNUM(N, Lo, Hi); break;
@@ -1966,6 +1967,14 @@ void DAGTypeLegalizer::ExpandFloatRes_FNEG(SDNode *N, SDValue &Lo,
Hi = DAG.getNode(ISD::FNEG, dl, Hi.getValueType(), Hi);
}
+void DAGTypeLegalizer::ExpandFloatRes_AssertNoFPClass(SDNode *N, SDValue &Lo,
+ SDValue &Hi) {
+ SDLoc dl(N);
+ GetExpandedFloat(N->getOperand(0), Lo, Hi);
+ Lo = DAG.getNode(ISD::AssertNoFPClass, dl, Lo.getValueType(), Lo);
+ Hi = DAG.getNode(ISD::AssertNoFPClass, dl, Hi.getValueType(), Hi);
+}
+
void DAGTypeLegalizer::ExpandFloatRes_FP_EXTEND(SDNode *N, SDValue &Lo,
SDValue &Hi) {
EVT NVT = TLI.getTypeToTransformTo(*DAG.getContext(), N->getValueType(0));
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
index 63544e63e1da1..33fa3012618b3 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
@@ -681,6 +681,7 @@ class LLVM_LIBRARY_VISIBILITY DAGTypeLegalizer {
SDNode *N, RTLIB::Libcall LC, std::optional<unsigned> CallRetResNo = {});
// clang-format off
+ void ExpandFloatRes_AssertNoFPClass(SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FABS (SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FACOS (SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FASIN (SDNode *N, SDValue &Lo, SDValue &Hi);
diff --git a/llvm/test/CodeGen/PowerPC/nofpclass.ll b/llvm/test/CodeGen/PowerPC/nofpclass.ll
new file mode 100644
index 0000000000000..444e62cbbcffc
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/nofpclass.ll
@@ -0,0 +1,13 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s \
+; RUN: | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-ibm-aix-xcoff < %s \
+; RUN: | FileCheck %s
+
+define ppc_fp128 @f(ppc_fp128 nofpclass(nan) %s) {
+; CHECK-LABEL: f:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: blr
+entry:
+ ret ppc_fp128 %s
+}
|
@llvm/pr-subscribers-llvm-selectiondag Author: Amy Kwan (amy-kwan) Changes780054d added support for This ISD node can be used with the
Thus, this patch aims to add support for the expand so we no longer assert. This fixes #151375. Full diff: https://github.com/llvm/llvm-project/pull/152357.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
index 2cad36eff9c88..8140c0e771a7b 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
@@ -1551,6 +1551,7 @@ void DAGTypeLegalizer::ExpandFloatResult(SDNode *N, unsigned ResNo) {
case ISD::VAARG: ExpandRes_VAARG(N, Lo, Hi); break;
case ISD::ConstantFP: ExpandFloatRes_ConstantFP(N, Lo, Hi); break;
+ case ISD::AssertNoFPClass: ExpandFloatRes_AssertNoFPClass(N, Lo, Hi); break;
case ISD::FABS: ExpandFloatRes_FABS(N, Lo, Hi); break;
case ISD::STRICT_FMINNUM:
case ISD::FMINNUM: ExpandFloatRes_FMINNUM(N, Lo, Hi); break;
@@ -1966,6 +1967,14 @@ void DAGTypeLegalizer::ExpandFloatRes_FNEG(SDNode *N, SDValue &Lo,
Hi = DAG.getNode(ISD::FNEG, dl, Hi.getValueType(), Hi);
}
+void DAGTypeLegalizer::ExpandFloatRes_AssertNoFPClass(SDNode *N, SDValue &Lo,
+ SDValue &Hi) {
+ SDLoc dl(N);
+ GetExpandedFloat(N->getOperand(0), Lo, Hi);
+ Lo = DAG.getNode(ISD::AssertNoFPClass, dl, Lo.getValueType(), Lo);
+ Hi = DAG.getNode(ISD::AssertNoFPClass, dl, Hi.getValueType(), Hi);
+}
+
void DAGTypeLegalizer::ExpandFloatRes_FP_EXTEND(SDNode *N, SDValue &Lo,
SDValue &Hi) {
EVT NVT = TLI.getTypeToTransformTo(*DAG.getContext(), N->getValueType(0));
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
index 63544e63e1da1..33fa3012618b3 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
@@ -681,6 +681,7 @@ class LLVM_LIBRARY_VISIBILITY DAGTypeLegalizer {
SDNode *N, RTLIB::Libcall LC, std::optional<unsigned> CallRetResNo = {});
// clang-format off
+ void ExpandFloatRes_AssertNoFPClass(SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FABS (SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FACOS (SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FASIN (SDNode *N, SDValue &Lo, SDValue &Hi);
diff --git a/llvm/test/CodeGen/PowerPC/nofpclass.ll b/llvm/test/CodeGen/PowerPC/nofpclass.ll
new file mode 100644
index 0000000000000..444e62cbbcffc
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/nofpclass.ll
@@ -0,0 +1,13 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s \
+; RUN: | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-ibm-aix-xcoff < %s \
+; RUN: | FileCheck %s
+
+define ppc_fp128 @f(ppc_fp128 nofpclass(nan) %s) {
+; CHECK-LABEL: f:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: blr
+entry:
+ ret ppc_fp128 %s
+}
|
Lo = DAG.getNode(ISD::AssertNoFPClass, dl, Lo.getValueType(), Lo); | ||
Hi = DAG.getNode(ISD::AssertNoFPClass, dl, Hi.getValueType(), Hi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's safe to directly take the value for both halves, and if it is it's only in double double cases like ppcf128. For ppcf128, it might not even apply to both halves. The safest option to get the compilation to stop failing is to drop the operation altogether and revisit the details of when it's preservable later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at PPCTargetLowering::LowerIS_FPCLASS, it appears that for ppcf128 the class is determined by element 1. So I think propagating the info for element 1 is correct. I don't have any evidence about element 0, so I think it should not be propagated there. I am okay with not propagating the assert for both elements for now. I don't see any type tests or asserts so it's hard to be confident that only ppcf128 reaches here, so I think ppcf128 specific stuff like element 1 is class should be if protected and there be a general clear both path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, I noticed I have a typo here.
As Roland mentioned, the class comes from the first element, which is what I meant:
SDValue PPCTargetLowering::LowerIS_FPCLASS(SDValue Op,
SelectionDAG &DAG) const {
assert(Subtarget.hasP9Vector() && "Test data class requires Power9");
SDValue LHS = Op.getOperand(0);
uint64_t RHSC = Op.getConstantOperandVal(1);
SDLoc Dl(Op);
FPClassTest Category = static_cast<FPClassTest>(RHSC);
if (LHS.getValueType() == MVT::ppcf128) {
// The higher part determines the value class.
LHS = DAG.getNode(ISD::EXTRACT_ELEMENT, Dl, MVT::f64, LHS,
DAG.getConstant(1, Dl, MVT::i32));
}
return getDataClassTest(LHS, Category, Dl, DAG, Subtarget);
}
Unless I am misunderstanding, based on the two comments, propagating the information from element 1 is correct but it sounds like should just drop the operation for now.
@arsenm Dumb question, sorry - but what exactly does dropping the operation look like in this case? I assume the Hi and Lo would not be set then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing through the results of GetExpandedFloat directly to the final results, without constructing a new AssertNoFPClass
; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s \ | ||
; RUN: | FileCheck %s | ||
; RUN: llc -verify-machineinstrs -mtriple=powerpc64-ibm-aix-xcoff < %s \ | ||
; RUN: | FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s \ | |
; RUN: | FileCheck %s | |
; RUN: llc -verify-machineinstrs -mtriple=powerpc64-ibm-aix-xcoff < %s \ | |
; RUN: | FileCheck %s | |
; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s | |
; RUN: llc -mtriple=powerpc64-ibm-aix-xcoff < %s | FileCheck %s |
; CHECK-NEXT: blr | ||
entry: | ||
ret ppc_fp128 %s | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally would test with more masks, and some cases that demonstrate downstream effects of preserving it after legalization. That's best left to a follow up that tries to preserve it though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be misunderstanding, but do you mean we are OK with updating the test in a follow up patch if we do decide not to drop the AssertNoFPClass when expanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a later patch which doesn't drop the assert would need more tests.
This is the minimal test which shows the legalization doesn't crash, but this doesn't show it's being preserved. You'd need some optimization that triggers on the type legalized dag, not sure what that would look like in the ppc case
Lo = DAG.getNode(ISD::AssertNoFPClass, dl, Lo.getValueType(), Lo); | ||
Hi = DAG.getNode(ISD::AssertNoFPClass, dl, Hi.getValueType(), Hi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing through the results of GetExpandedFloat directly to the final results, without constructing a new AssertNoFPClass
SDValue &Hi) { | ||
SDLoc dl(N); | ||
GetExpandedFloat(N->getOperand(0), Lo, Hi); | ||
Lo = DAG.getNode(ISD::AssertNoFPClass, dl, Lo.getValueType(), Lo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lo = DAG.getNode(ISD::AssertNoFPClass, dl, Lo.getValueType(), Lo); |
56854a1
to
c2ae546
Compare
void DAGTypeLegalizer::ExpandFloatRes_AssertNoFPClass(SDNode *N, SDValue &Lo, | ||
SDValue &Hi) { | ||
SDLoc dl(N); | ||
GetExpandedFloat(N->getOperand(0), Lo, Hi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add todo to preserve it for the half for ppcf128 if you aren't going to immediately follow up with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I've added the TODO here and in the test.
; CHECK-NEXT: blr | ||
entry: | ||
ret ppc_fp128 %s | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a later patch which doesn't drop the assert would need more tests.
This is the minimal test which shows the legalization doesn't crash, but this doesn't show it's being preserved. You'd need some optimization that triggers on the type legalized dag, not sure what that would look like in the ppc case
780054d added support for `ISD::AssertNoFPClass`. This ISD node can be used with the `ppc_fp128` type, which is really just two `f64s` and requires expanding when used with `ISD::AssertNoFPClass`. Without the support for expanding the result, we get an assertion because the legalizer does not know how to expand the results of `ppc_fp128` with `ISD::AssertNoFPClass`. ``` ExpandFloatResult #0: t7: ppcf128 = AssertNoFPClass t5, TargetConstant:i32<3> LLVM ERROR: Do not know how to expand the result of this operator! ``` Thus, this patch aims to add support for the expand so we no longer assert. This fixes llvm#151375.
388fce6
to
0362e0d
Compare
/cherry-pick 63cc2e3 |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/23502 Here is the relevant piece of the build log for the reference
|
I've reverted my change locally. This failure doesn't appear to be due to my change. |
780054d added support for
ISD::AssertNoFPClass
.This ISD node can be used with the
ppc_fp128
type, which is really just twof64s
and requires expanding when used withISD::AssertNoFPClass
. Without the support for expanding the result, we get an assertion because the legalizer does not know how to expand the results ofppc_fp128
withISD::AssertNoFPClass
.Thus, this patch aims to add support for the expand so we no longer assert.
This fixes #151375.